#9588: Add support to multi-band color mapping for COG layers #9857
Conversation
|
Looks like GitHub experiencing outage causing actions to fails |
There was a problem hiding this comment.
- information for bands and styling should be documented, or is not easy to understand if they are properly made to be compatible for future evolution. Note that style information should be compatible with a future expansion with multiple source (so store style and source information with this in mind). I'll ask to @dromagnoli some help understanding these parameters
For the moment here some bugs I found:
-
once changed, there is no way to restore max and min
screencast-mail.google.com-2024.01.16-16_55_16.webm
-
Sometimes it switches to one band randomly
screencast-mail.google.com-2024.01.16-17_03_21.webm
|
Hi, from a brief discussion about it with @dromagnoli I understood that Ideally I'd transfer to the layer the information about the tiff as plain as possible to the layer metadata, so if there is some error in interpretation, we can fix the code and we don't have to fix the layer. Theoretically even if data can suggest you the style to use, you may decide to show 1 gray scale, RGB or RGBA independently from the origial bands list. But let's decide with tobia if this should be added now or later. Notice that photometric interpretation can be So I suggest to :
From my understanding if samples are 1, 3 (or 4, adding the extra one) you may desume the number of "bands" so you can correctly initialize the band selector. |
This is because, on In the future enhancement, the fetch metadata operation can also be performed from layer settings ex: style editor, which should help us handle this case. For now, can I just add a warning message in Style Editor saying Apart from this, the rest is ready for review. Thanks! |
There was a problem hiding this comment.
I understood that you want to allow min/max source data but editing it in the styler looks uncorrect.
Other stylers allow to edit min/max per band, for instance QGIS
I noticed you provided a tooltip to clarify it but it could not be enough. let me take a look at this together with @tdipisa before to see how to proceed.
- In particular I noticed that these values are anyway applied, even if I deactivate the layer.
screencast-mail.google.com-2024.01.22-11_58_19.webm
I understand the reason but from the user experience this is very confusing.
- saving whole
fileDirectorylooks to be potentially an issue. It contains a lot of data that may potentially make the JSON explode (seeTileOffsets,TileByteCounts,GeoKeyDirectory). Please fix and save only the data you effectively need, not relying on them. - Save the style in a format compatible with other stylers. Here you are storing it as a string, but it should be ajson. Moreover it should be useful to make it consistent with the other style formats.
{ "style": { "body": {
"color": ["array",["band",1],["band",2],["band",3],["band",4]]"
}, "format": "openlayers"
- please document the raster style in md doc too.
|
confirmed the format with @allyoucanmap , let me talk about min and max with tobia. |
From the video attached, I see the band styling is only disabled, meaning the bands selected from the channels will be ignored but not min and max, as these are independent. Min/max can be cleared if the user chooses to apply only source data values. Additionally, there are cases where only either is needed, for this reason, I have provided an option to disable band styling separately.
My thoughts exactly and I wanted to discuss when PR is reviewed. Glad you noticed. I will add only the |
tdipisa
added
the
BackportNeeded
|
@dsuren1 can you please fix conflicts? |
|
Resolved |
|
I performed the tests on this map: http://localhost:8081/#/context/COG/46403 Here my findings: Bugs
screencast-mail.google.com-2024.03.14-18_01_02.webmThings to discussIt is not easy to review the PR because of this regression not related to the PR (verified also on master): If I add layers to the map, now I don't have the information for zoom etc.. anymore. |
I think it is better if you open a dedicated issue to report the regression you have identified with proper steps to reproduce it, mentioning also if it is affecting only master or also the release. |
The problem is caused by missing metadata as explained here |
There was a problem hiding this comment.
We are almost in a stable version I think. We have to fix the following:
- "enableBandStyling" property should not be used, but the style should depend only on the style object. I think the presence of the body.color section is enough to determine if it is enabled or not, but if you need more, add a flag and proper document it.
- For certain cogs is still unclear to me how to style. For instance this https://cogeo.craig.fr/opendata/ortho/2018_25cm_ain+isere.tif the styler do not look to be working at all. Maybe because of photometryic interpretation : 6 ?
a final note:
Having to save catalogs again and again is counter-intuitive at the end. If we need more metadata, we need to think about a way to update/auto-update layers source metadata.
You are right. This prop is not needed. Might have skipped the coffee 😉
At times, the alpha channel may prevent the visualization of certain TIFF sources, and the composition of some TIFF sources may disregard the alpha channel. To mitigate this issue and to eliminate no-data tiles surrounding the image, I have incorporated the nodata attribute into the source (TODO: should made configurable in the future enhancement), which introduces an alpha channel and facilitates the better visualization of the TIFF image. 2018_25cm_ain+isere.tif
At least after this change, all the required metadatas are fetched and persisted. But yes, maybe a force fetch metadata option somewhere in the layer settings is needed in the future. |
There was a problem hiding this comment.
At least after this change, all the required metadatas are fetched and persisted. But yes, maybe a force fetch metadata option somewhere in the layer settings is needed in the future.
I think it might be nice to have it soon, maybe adding a layer refresh button in the toolbar valid for all levels which for COGs also updates the level metadata.
|
@ElenaGallo, could you please test this on DEV ? Thank you |
|
Test passed, @dsuren1 please backport to 2024.01.xx. Thanks |
… layers (geosolutions-it#9857) (cherry picked from commit d683b82)
tdipisa
removed
the
BackportNeeded
Description
This PR allows multi band color mapping of COG layers along modification of min and max source data value
Please check if the PR fulfills these requirements
What kind of change does this PR introduce? (check one with "x", remove the others)
Issue
What is the current behavior?
What is the new behavior?
Breaking change
Does this PR introduce a breaking change? (check one with "x", remove the other)
Other useful information